Fix `auth profiles` misclassifying SPOG hosts as workspace configs by mihaimitrea-db · Pull Request #4929 · databricks/cli

mihaimitrea-db · 2026-04-10T13:20:20Z

Summary

The SDK's ConfigType() classifies hosts by URL prefix (accounts.* → account, everything else → workspace). SPOG hosts don't match the accounts.* prefix, so they were misclassified as WorkspaceConfig and validated with CurrentUser.Me, which fails on account-scoped SPOG hosts.
Use the resolved DiscoveryURL from /.well-known/databricks-config to detect SPOG hosts with account-scoped OIDC (contains /oidc/accounts/), matching the routing logic in auth.AuthArguments.ToOAuthArgument() and the approach from Fix auth logout failing to clear token for workspace profiles with account ID #4853.
Add a fallback for legacy profiles with experimental_is_unified_host where .well-known is unreachable.

Why not just check `account_id`?

Since #4809, runHostDiscovery populates account_id on every workspace profile from the .well-known endpoint. A regular workspace profile now routinely carries account_id. The only reliable discriminator is the oidc_endpoint shape from .well-known, resolved at runtime (as established in #4853).

Test plan

Unit tests: TestProfileLoadSPOGConfigType — table-driven with mock HTTP servers covering SPOG account, SPOG workspace, SPOG with workspace_id=none, and classic workspace with discovery-populated account_id.
Unit test: TestProfileLoadUnifiedHostFallback — experimental_is_unified_host profile with unreachable .well-known falls back to account validation.
Unit test: TestProfileLoadClassicAccountHost — classic account-scoped OIDC host.
Acceptance test: cmd/auth/profiles/spog-account — end-to-end: SPOG profile with workspace_id=none shows valid:true.
go test ./cmd/auth and go test ./acceptance -run TestAccept/cmd/auth/profiles pass.

SPOG hosts (e.g. db-deco-test.gcp.databricks.com) don't match the accounts.* URL prefix, so ConfigType() classifies them as WorkspaceConfig. This causes `auth profiles` to validate with CurrentUser.Me instead of Workspaces.List, which fails for account-scoped SPOG profiles. Use the resolved DiscoveryURL from .well-known/databricks-config to detect SPOG hosts with account-scoped OIDC, matching the routing logic in auth.AuthArguments.ToOAuthArgument(). Also add a fallback for legacy profiles with Experimental_IsUnifiedHost where .well-known is unreachable.

simonfaltum

Review (multi-agent swarm: Isaac + Cursor, cross-reviewed)

Overall the fix is correct and well-targeted. The routing logic properly mirrors ToOAuthArgument(), and the tests are thorough. A few suggestions below, nothing blocking.

Summary of findings

#	Severity	Finding	File
1	suggestion	Legacy fallback ignores `workspace_id`	`profiles.go:77-82`
2	suggestion	Logic duplication with `arguments.go`	`profiles.go:61-75`
3	suggestion	Misleading test name	`profiles_test.go:224`
4	nitpick	Tests don't prove which validation branch ran	`profiles_test.go:85`
5	nitpick	Unused `wantConfigCloud` test field	`profiles_test.go:123`
6	suggestion	Missing negative test case	`profiles_test.go`
7	pass	Go code structure checklist	all files

See inline comments for details.

simonfaltum · 2026-04-13T10:12:51Z

+	// Legacy backward compat: profiles with Experimental_IsUnifiedHost where
+	// .well-known is unreachable (so DiscoveryURL is empty). Matches the
+	// fallback in auth.AuthArguments.ToOAuthArgument().
+	if configType == config.InvalidConfig && cfg.Experimental_IsUnifiedHost && cfg.AccountID != "" {
+		configType = config.AccountConfig
+	}


suggestion: This legacy fallback always forces AccountConfig regardless of workspace_id. In arguments.go:80-81, ignoring workspace_id is intentional because unified OAuth routing is always account-scoped. But here you're choosing a validation strategy, not an OAuth flow. MatchWorkspaceProfiles in profiler.go treats any non-none workspace_id as workspace-scoped.

If a legacy experimental_is_unified_host profile has a real workspace_id and .well-known is unreachable, this would validate it with Workspaces.List instead of CurrentUser.Me. That said, the combination of Experimental_IsUnifiedHost + real workspace_id is unlikely in practice since workspace-scoped SPOG came after the legacy flag era.

Consider either:

Adding the same workspace_id branching here as in the discovery block above, or

Adding a comment explaining why it's intentionally omitted

simonfaltum · 2026-04-13T10:12:51Z

+	configType := cfg.ConfigType()
+	isAccountScopedOIDC := cfg.DiscoveryURL != "" && strings.Contains(cfg.DiscoveryURL, "/oidc/accounts/")
+	if configType != config.AccountConfig && cfg.AccountID != "" && isAccountScopedOIDC {
+		if cfg.WorkspaceID != "" && cfg.WorkspaceID != auth.WorkspaceIDNone {
+			configType = config.WorkspaceConfig
+		} else {
+			configType = config.AccountConfig
+		}
+	}


suggestion (follow-up): The SPOG detection heuristic (DiscoveryURL contains /oidc/accounts/ + IsUnifiedHost fallback) is now duplicated between here and libs/auth/arguments.go:69-82. The cross-referencing comments help, but if the routing logic changes in one place the other can drift.

Consider extracting a shared helper in libs/auth/ in a follow-up, e.g.:

func ResolveConfigType(cfg *config.Config) config.ConfigType { ... }

Not blocking for this PR since the logic is simple and well-documented.

This would be good to have. Bonus points that it moves some of the logic form the SDK to the CLI.

simonfaltum · 2026-04-13T10:12:51Z

+}
+
+func TestProfileLoadClassicAccountHost(t *testing.T) {
+	// Verify that a host with account-scoped OIDC from discovery is validated


suggestion: This test name says "classic account host" but the httptest.Server URL (e.g. http://127.0.0.1:PORT) doesn't match the accounts.* prefix, so ConfigType() won't return AccountConfig for the classic-host reason. This actually exercises the SPOG discovery override path (same as TestProfileLoadSPOGConfigType).

Consider renaming to something like TestProfileLoadSPOGAccountWithDiscovery, or updating the comment to acknowledge that classic accounts.* behavior can't easily be tested with httptest and this serves as a supplementary SPOG case.

simonfaltum · 2026-04-13T10:12:51Z

+// newProfileTestServer creates a mock server for profile validation tests.
+// It serves /.well-known/databricks-config with the given OIDC shape and
+// responds to the workspace/account validation API endpoints.
+func newProfileTestServer(t *testing.T, accountScoped bool, accountID string) *httptest.Server {


nitpick: Both the workspace and account validation endpoints (/api/2.0/preview/scim/v2/Me and /api/2.0/accounts/{id}/workspaces) return success, so Valid == true doesn't prove which branch was taken. Consider making the "wrong" endpoint return an error for each test case to prove routing correctness.

suggestion: Also consider adding a negative test case where .well-known returns 404, account_id is set, Experimental_IsUnifiedHost is false, to verify the profile stays as WorkspaceConfig (proving the override doesn't trigger accidentally).

- Add workspace_id branching to legacy IsUnifiedHost fallback - Rename TestProfileLoadClassicAccountHost to TestProfileLoadSPOGAccountWithDiscovery - Extract hasWorkspace variable to deduplicate condition - Add negative test: no discovery + account_id stays WorkspaceConfig - Fix misleading mock server comments

simonfaltum

Looks good! Clean fix that correctly mirrors the routing logic from ToOAuthArgument().

A few minor observations:

The /oidc/accounts/ substring check now lives in both profiles.go and arguments.go. Worth extracting a shared helper (or at least a constant) if a third consumer appears.
A log.Debugf when the config type gets overridden would help with troubleshooting, consistent with the existing profile load timing log.
TestProfileLoadSPOGAccountWithDiscovery covers the same path as the table-driven case "SPOG account profile validated as account". Could remove it or repurpose it to test a classic accounts.* host.
The hasWorkspace branch in the SPOG override is a no-op when ConfigType() already returns WorkspaceConfig. Could simplify to only fire when !hasWorkspace, or add a brief comment.

- Add log.Debugf when config type is overridden (review comment #2) - Remove redundant TestProfileLoadSPOGAccountWithDiscovery, replace with TestClassicAccountsHostConfigType (review comment #3)

## Summary - Extract the SPOG detection heuristic into a shared `IsSPOG(cfg, accountID)` predicate in `libs/auth/config_type.go`, replacing inline checks in both `profiles.go` and `ToOAuthArgument`. - Extract `ResolveConfigType(cfg)` that wraps `ConfigType()` with SPOG-aware overrides, used by `profiles.go`. - `ToOAuthArgument` now calls `IsSPOG` instead of duplicating the `DiscoveryURL` + `IsUnifiedHost` checks inline. Follow-up to #4929 as suggested in review comment #2 (logic duplication). ### Note on IsUnifiedHost fallback scope The `IsSPOG` predicate checks `Experimental_IsUnifiedHost` unconditionally (not gated on `configType == InvalidConfig`). This is broader than the previous inline check in `profiles.go`, which only fired for `InvalidConfig`. This is safe because the SDK currently returns `InvalidConfig` for all unified hosts (the `UnifiedHost` case was removed from `ConfigType()` in v0.126.0). It is also more robust against future SDK changes that might reclassify unified hosts differently. ## Test plan - [x] `TestResolveConfigType` — 9-case table-driven unit test in `libs/auth/config_type_test.go`. - [x] All existing `TestProfileLoad*` and `TestToOAuthArgument*` tests pass. - [x] `go test ./libs/auth/ ./cmd/auth/` passes.

mihaimitrea-db added 2 commits April 10, 2026 13:13

Add acceptance test for SPOG account profile validation

7b8d122

mihaimitrea-db marked this pull request as ready for review April 10, 2026 13:25

mihaimitrea-db requested review from andrewnester, anton-107, denik, pietern, shreyas-goenka and simonfaltum as code owners April 10, 2026 13:25

Use tagged switch to fix staticcheck QF1002 lint errors

456280f

simonfaltum removed request for andrewnester, anton-107, denik, pietern and shreyas-goenka April 10, 2026 13:27

Clean up test: remove unused field, fix misleading comment

a7198ac

simonfaltum reviewed Apr 13, 2026

View reviewed changes

mihaimitrea-db and others added 2 commits April 13, 2026 10:51

Merge branch 'main' into mihaimitrea-db/fix-profiles-spog-classification

05c4f39

mihaimitrea-db mentioned this pull request Apr 13, 2026

Extract IsSPOG and ResolveConfigType into libs/auth #4939

Merged

3 tasks

mihaimitrea-db requested a review from simonfaltum April 13, 2026 11:18

simonfaltum approved these changes Apr 14, 2026

View reviewed changes

Address review comments on profiles fix

2ae8ec0

- Add log.Debugf when config type is overridden (review comment #2) - Remove redundant TestProfileLoadSPOGAccountWithDiscovery, replace with TestClassicAccountsHostConfigType (review comment #3)

mihaimitrea-db enabled auto-merge April 14, 2026 15:34

mihaimitrea-db added this pull request to the merge queue Apr 14, 2026

Merged via the queue into main with commit f86c804 Apr 14, 2026
22 checks passed

mihaimitrea-db deleted the mihaimitrea-db/fix-profiles-spog-classification branch April 14, 2026 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `auth profiles` misclassifying SPOG hosts as workspace configs#4929

Fix `auth profiles` misclassifying SPOG hosts as workspace configs#4929
mihaimitrea-db merged 7 commits intomainfrom
mihaimitrea-db/fix-profiles-spog-classification

mihaimitrea-db commented Apr 10, 2026 •

edited

Loading

Uh oh!

simonfaltum left a comment

Uh oh!

simonfaltum Apr 13, 2026

Uh oh!

simonfaltum Apr 13, 2026

Uh oh!

mihaimitrea-db Apr 13, 2026

Uh oh!

simonfaltum Apr 13, 2026

Uh oh!

Uh oh!

simonfaltum Apr 13, 2026

Uh oh!

simonfaltum left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mihaimitrea-db commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why not just check account_id?

Test plan

Uh oh!

simonfaltum left a comment

Choose a reason for hiding this comment

Review (multi-agent swarm: Isaac + Cursor, cross-reviewed)

Summary of findings

Uh oh!

simonfaltum Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

simonfaltum Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

mihaimitrea-db Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

simonfaltum Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonfaltum Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

simonfaltum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mihaimitrea-db commented Apr 10, 2026 •

edited

Loading

Why not just check `account_id`?